pull: complete detached meta fetch before scanning

If somehow a repo has gpg verification on but doesn't have signatures
present for the existing commit, ostree would error out if it needs to
scan the commit object (e.g. if there are no updates available).

An instance of this is currently happening in Fedora AH, in which
signatures are not shipped in the ISO due to filesystem restrictions.
Another possible scenario is if a content provider switches from not
signing commits to signing them; even if older commits are retroactively
signed, clients' local commit objects would error out if they needed
scanning.

This patch adds a check to ensure that we always attempt to fetch the
detached metadata and wait for its result (whether it exists or not)
before moving on to scan their corresponding commit objects.

See also: https://github.com/projectatomic/rpm-ostree/issues/630

Closes: #873
Approved by: cgwalters

diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c
index f1dae995b40b4002ed11c9ad3af5a971395c350c..08692c59eebfc1ce2bca48af246c007364b19510 100644 (file)
--- a/src/libostree/ostree-repo-pull.c
+++ b/src/libostree/ostree-repo-pull.c
@@ -80,6 +80,7 @@ typedef struct {
   GHashTable       *expected_commit_sizes; /* Maps commit checksum to known size */
   GHashTable       *commit_to_depth; /* Maps commit checksum maximum depth */
   GHashTable       *scanned_metadata; /* Maps object name to itself */
+  GHashTable       *fetched_detached_metadata; /* Set<checksum> */
   GHashTable       *requested_metadata; /* Maps object name to itself */
   GHashTable       *requested_content; /* Maps checksum to itself */
   GHashTable       *requested_fallback_content; /* Maps checksum to itself */
@@ -912,8 +913,15 @@ meta_fetch_on_complete (GObject           *object,
             {
               /* There isn't any detached metadata, just fetch the commit */
               g_clear_error (&local_error);
+
+              /* Now that we've at least tried to fetch it, we can proceed to
+               * scan/fetch the commit object */
+              g_hash_table_add (pull_data->fetched_detached_metadata, g_strdup (checksum));
+
               if (!fetch_data->object_is_stored)
                 enqueue_one_object_request (pull_data, checksum, objtype, fetch_data->path, FALSE, FALSE);
+              else
+                queue_scan_one_metadata_object (pull_data, checksum, objtype, fetch_data->path, 0);
             }
 
           /* When traversing parents, do not fail on a missing commit.
@@ -960,8 +968,12 @@ meta_fetch_on_complete (GObject           *object,
                                                        pull_data->cancellable, error))
         goto out;
 
+      g_hash_table_add (pull_data->fetched_detached_metadata, g_strdup (checksum));
+
       if (!fetch_data->object_is_stored)
         enqueue_one_object_request (pull_data, checksum, objtype, fetch_data->path, FALSE, FALSE);
+      else
+        queue_scan_one_metadata_object (pull_data, checksum, objtype, fetch_data->path, 0);
     }
   else
     {
@@ -977,7 +989,7 @@ meta_fetch_on_complete (GObject           *object,
           if (!write_commitpartial_for (pull_data, checksum, error))
             goto out;
         }
-      
+
       ostree_repo_write_metadata_async (pull_data->repo, objtype, checksum, metadata,
                                         pull_data->cancellable,
                                         on_metadata_written, fetch_data);
@@ -1377,15 +1389,20 @@ scan_one_metadata_object_c (OtPullData         *pull_data,
     }
   else if (is_stored && objtype == OSTREE_OBJECT_TYPE_COMMIT)
     {
-      /* For commits, always refetch detached metadata. */
-      enqueue_one_object_request (pull_data, tmp_checksum, objtype, path, TRUE, TRUE);
-
-      if (!scan_commit_object (pull_data, tmp_checksum, recursion_depth,
-                               pull_data->cancellable, error))
-        goto out;
+      /* Even though we already have the commit, we always try to (re)fetch the
+       * detached metadata before scanning it, in case new signatures appear.
+       * https://github.com/projectatomic/rpm-ostree/issues/630 */
+      if (!g_hash_table_contains (pull_data->fetched_detached_metadata, tmp_checksum))
+        enqueue_one_object_request (pull_data, tmp_checksum, objtype, path, TRUE, TRUE);
+      else
+        {
+          if (!scan_commit_object (pull_data, tmp_checksum, recursion_depth,
+                                   pull_data->cancellable, error))
+            goto out;
 
-      g_hash_table_add (pull_data->scanned_metadata, g_variant_ref (object));
-      pull_data->n_scanned_metadata++;
+          g_hash_table_add (pull_data->scanned_metadata, g_variant_ref (object));
+          pull_data->n_scanned_metadata++;
+        }
     }
   else if (is_stored && objtype == OSTREE_OBJECT_TYPE_DIR_TREE)
     {
@@ -2787,6 +2804,8 @@ ostree_repo_pull_with_options (OstreeRepo             *self,
                                                                (GDestroyNotify)g_free);
   pull_data->scanned_metadata = g_hash_table_new_full (ostree_hash_object_name, g_variant_equal,
                                                        (GDestroyNotify)g_variant_unref, NULL);
+  pull_data->fetched_detached_metadata = g_hash_table_new_full (g_str_hash, g_str_equal,
+                                                       (GDestroyNotify)g_free, NULL);
   pull_data->requested_content = g_hash_table_new_full (g_str_hash, g_str_equal,
                                                         (GDestroyNotify)g_free, NULL);
   pull_data->requested_fallback_content = g_hash_table_new_full (g_str_hash, g_str_equal,
@@ -3509,6 +3528,7 @@ ostree_repo_pull_with_options (OstreeRepo             *self,
   g_clear_pointer (&pull_data->commit_to_depth, (GDestroyNotify) g_hash_table_unref);
   g_clear_pointer (&pull_data->expected_commit_sizes, (GDestroyNotify) g_hash_table_unref);
   g_clear_pointer (&pull_data->scanned_metadata, (GDestroyNotify) g_hash_table_unref);
+  g_clear_pointer (&pull_data->fetched_detached_metadata, (GDestroyNotify) g_hash_table_unref);
   g_clear_pointer (&pull_data->summary_deltas_checksums, (GDestroyNotify) g_hash_table_unref);
   g_clear_pointer (&pull_data->requested_content, (GDestroyNotify) g_hash_table_unref);
   g_clear_pointer (&pull_data->requested_fallback_content, (GDestroyNotify) g_hash_table_unref);

diff --git a/tests/pull-test.sh b/tests/pull-test.sh
index f81d8023fade5e037bdb2f983aac70602b15a989..3a836da9a1c973705deb19dc3711d67e5ceca5a9 100644 (file)
--- a/tests/pull-test.sh
+++ b/tests/pull-test.sh
@@ -24,7 +24,7 @@ function repo_init() {
     rm repo -rf
     mkdir repo
     ostree_repo_init repo
-    ${CMD_PREFIX} ostree --repo=repo remote add --set=gpg-verify=false origin $(cat httpd-address)/ostree/gnomerepo
+    ${CMD_PREFIX} ostree --repo=repo remote add origin $(cat httpd-address)/ostree/gnomerepo "$@"
 }
 
 function verify_initial_contents() {
@@ -35,10 +35,10 @@ function verify_initial_contents() {
     assert_file_has_content baz/cow '^moo$'
 }
 
-echo "1..16"
+echo "1..18"
 
 # Try both syntaxes
-repo_init
+repo_init --no-gpg-verify
 ${CMD_PREFIX} ostree --repo=repo pull origin main
 ${CMD_PREFIX} ostree --repo=repo pull origin:main
 ${CMD_PREFIX} ostree --repo=repo fsck
@@ -128,7 +128,7 @@ assert_file_has_content main.txt ${rev}
 echo "ok pull specific commit"
 
 cd ${test_tmpdir}
-repo_init
+repo_init --no-gpg-verify
 ${CMD_PREFIX} ostree --repo=repo pull origin main
 ${CMD_PREFIX} ostree --repo=repo fsck
 # Generate a delta from old to current, even though we aren't going to
@@ -153,7 +153,7 @@ ${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo summary -u
 # Explicitly test delta fetches via ref name as well as commit hash
 for delta_target in main ${new_rev}; do
 cd ${test_tmpdir}
-repo_init
+repo_init --no-gpg-verify
 ${CMD_PREFIX} ostree --repo=repo pull origin main@${prev_rev}
 ${CMD_PREFIX} ostree --repo=repo pull --dry-run --require-static-deltas origin ${delta_target} >dry-run-pull.txt
 # Compression can vary, so we support 400-699
@@ -166,7 +166,7 @@ done
 # Explicitly test delta fetches via ref name as well as commit hash
 for delta_target in main ${new_rev}; do
 cd ${test_tmpdir}
-repo_init
+repo_init --no-gpg-verify
 ${CMD_PREFIX} ostree --repo=repo pull origin main@${prev_rev}
 ${CMD_PREFIX} ostree --repo=repo pull --require-static-deltas origin ${delta_target}
 if test ${delta_target} = main; then
@@ -179,7 +179,7 @@ ${CMD_PREFIX} ostree --repo=repo fsck
 done
 
 cd ${test_tmpdir}
-repo_init
+repo_init --no-gpg-verify
 ${CMD_PREFIX} ostree --repo=repo pull origin main@${prev_rev}
 ${CMD_PREFIX} ostree --repo=repo pull --disable-static-deltas origin main
 ${CMD_PREFIX} ostree --repo=repo fsck
@@ -197,7 +197,7 @@ cd ${test_tmpdir}
 ${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo static-delta generate --swap-endianness main
 ${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo summary -u
 
-repo_init
+repo_init --no-gpg-verify
 ${CMD_PREFIX} ostree --repo=repo pull origin main@${prev_rev}
 ${CMD_PREFIX} ostree --repo=repo pull --require-static-deltas --dry-run origin main >byteswapped-dry-run-pull.txt
 ${CMD_PREFIX} ostree --repo=repo fsck
@@ -211,7 +211,7 @@ echo "ok pull byteswapped delta"
 cd ${test_tmpdir}
 rm ostree-srv/gnomerepo/deltas -rf
 ${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo summary -u
-repo_init
+repo_init --no-gpg-verify
 if ${CMD_PREFIX} ostree --repo=repo pull --require-static-deltas origin main 2>err.txt; then
     assert_not_reached "--require-static-deltas unexpectedly succeeded"
 fi
@@ -219,7 +219,7 @@ assert_file_has_content err.txt "deltas required, but none found"
 ${CMD_PREFIX} ostree --repo=repo fsck
 
 # Now test with a partial commit
-repo_init
+repo_init --no-gpg-verify
 ${CMD_PREFIX} ostree --repo=repo pull --commit-metadata-only origin main@${prev_rev}
 if ${CMD_PREFIX} ostree --repo=repo pull --require-static-deltas origin main 2>err.txt; then
     assert_not_reached "--require-static-deltas unexpectedly succeeded"
@@ -227,7 +227,7 @@ fi
 assert_file_has_content err.txt "deltas required, but none found"
 echo "ok delta required but don't exist"
 
-repo_init
+repo_init --no-gpg-verify
 ${CMD_PREFIX} ostree --repo=repo pull origin main@${prev_rev}
 if ${CMD_PREFIX} ostree --repo=repo pull --require-static-deltas origin ${new_rev} 2>err.txt; then
     assert_not_reached "--require-static-deltas unexpectedly succeeded"
@@ -294,3 +294,27 @@ fi
 assert_file_has_content err.txt "ONE BILLION DOLLARS"
 
 echo "ok unconfigured"
+
+cd ${test_tmpdir}
+repo_init --set=gpg-verify=true
+${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo commit \
+  --gpg-homedir=${TEST_GPG_KEYHOME} --gpg-sign=${TEST_GPG_KEYID_1} -b main \
+  -s "A signed commit" --tree=ref=main
+${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo summary -u
+# make sure gpg verification is correctly on
+csum=$(${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo rev-parse main)
+objpath=objects/${csum::2}/${csum:2}.commitmeta
+remotesig=ostree-srv/gnomerepo/$objpath
+localsig=repo/$objpath
+mv $remotesig $remotesig.bak
+if ${CMD_PREFIX} ostree --repo=repo --depth=0 pull origin main; then
+    assert_not_reached "pull with gpg-verify unexpectedly succeeded?"
+fi
+# ok now check that we can pull correctly
+mv $remotesig.bak $remotesig
+${CMD_PREFIX} ostree --repo=repo pull origin main
+echo "ok pull signed commit"
+rm $localsig
+${CMD_PREFIX} ostree --repo=repo pull origin main
+test -f $localsig
+echo "ok re-pull signature for stored commit"

diff --git a/tests/test-remote-headers.sh b/tests/test-remote-headers.sh
index 6902cefb5c9ad5672d830fbcc11a186756ba9d50..39fbe352c1858a46d93f15e341c7749766d36386 100755 (executable)
--- a/tests/test-remote-headers.sh
+++ b/tests/test-remote-headers.sh
@@ -44,9 +44,9 @@ ${CMD_PREFIX} ostree --repo=repo remote add --set=gpg-verify=false origin $(cat
 # Sanity check the setup, without headers the pull should fail
 assert_fail ${CMD_PREFIX} ostree --repo=repo pull origin main
 
-echo "ok, setup done"
+echo "ok setup done"
 
 # Now pull should succeed now
 ${CMD_PREFIX} ostree --repo=repo pull --http-header foo=bar --http-header baz=badger origin main
 
-echo "ok, pull succeeded"
+echo "ok pull succeeded"